fix(repair): stop dropping drawers with zero embedding_metadata rows - #2087
Open
KeilerHirsch wants to merge 1 commit into
Open
fix(repair): stop dropping drawers with zero embedding_metadata rows#2087KeilerHirsch wants to merge 1 commit into
KeilerHirsch wants to merge 1 commit into
Conversation
extract_via_sqlite drove its extraction with an INNER JOIN starting at embedding_metadata. Any embedding with zero rows there (a sparse historical write with no chroma:document and no other key -- the same condition _extract_drawers already sanitizes for the collection-layer rebuild path, see MemPalace#1458) never appeared in the join result and was silently dropped by mempalace repair --mode from-sqlite, with no count mismatch and no warning. Drive the query from embeddings (LEFT JOIN embedding_metadata) instead, defaulting to an empty metadata dict when a row has none. Order by e.id rather than em.id since em.id is NULL for unmatched rows. Regression test seeds a real chromadb collection, then strips one drawer's metadata rows directly via SQLite (current chromadb validates against empty metadata on write, so this only reproduces on rows already sitting in an older palace) and asserts the drawer still comes back through the SQLite bypass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
extract_via_sqlite(the recovery pathrebuild_from_sqliteuses when the chromadb collection layer itself is broken, e.g. the#1308HNSW-segment failure) drives its extraction with anINNER JOINthat starts atembedding_metadata:Any embedding with zero rows in
embedding_metadata— a sparse historical write with nochroma:documentkey and no other key — never appears in the join result at all, so it is silently dropped.This condition is not hypothetical: the sibling extractor
_extract_drawers(used by the collection-layerrebuild_indexpath) already sanitizes for it directly in its own comments — "drawers extracted from sqlite ground truth can come back withNoneor{}for sparse historical writes" (#1458).extract_via_sqlitehas no equivalent handling, because such rows are structurally invisible to a join anchored onembedding_metadata.Running
mempalace repair --mode from-sqlite --archive-existingon a palace containing any such drawer drops it with no count mismatch and no warning.Fix
Drive the query from
embeddings(LEFT JOIN embedding_metadata) instead, defaulting to an empty metadata dict when a row has none. Order bye.idrather thanem.id, sinceem.idisNULLfor the now-included unmatched rows.Tests
One new regression test in
tests/test_repair.py: seeds a real chromadb collection via the existing_seed_palacehelper, then strips one drawer'sembedding_metadatarows directly via SQLite (current chromadb validates against empty metadata on write, so this only reproduces on rows already sitting in an older palace) and asserts the drawer still comes back through the SQLite bypass. Confirmed failing against the pre-fix code, passing after the fix. All 97 existing tests intests/test_repair.pystill pass.